Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Combined FSTree versioning and log #3031

Merged
merged 8 commits into from
Nov 28, 2024
Merged

Conversation

roman-khimov
Copy link
Member

No description provided.

@roman-khimov roman-khimov added this to the v0.44.0 milestone Nov 26, 2024
@roman-khimov roman-khimov force-pushed the combined-versioning-and-log branch from 825eb5f to dfa8068 Compare November 26, 2024 15:17
Copy link

codecov bot commented Nov 26, 2024

Codecov Report

Attention: Patch coverage is 40.00000% with 15 lines in your changes missing coverage. Please review.

Project coverage is 22.64%. Comparing base (edc95f6) to head (b79dcfa).
Report is 9 commits behind head on master.

Files with missing lines Patch % Lines
pkg/local_object_storage/blobstor/fstree/fstree.go 43.75% 7 Missing and 2 partials ⚠️
pkg/local_object_storage/blobstor/blobstor.go 0.00% 3 Missing ⚠️
pkg/local_object_storage/blobstor/peapod/peapod.go 0.00% 2 Missing ⚠️
...ject_storage/blobstor/fstree/fstree_write_linux.go 75.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3031      +/-   ##
==========================================
- Coverage   22.64%   22.64%   -0.01%     
==========================================
  Files         791      791              
  Lines       58618    58615       -3     
==========================================
- Hits        13274    13271       -3     
  Misses      44447    44447              
  Partials      897      897              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@roman-khimov roman-khimov force-pushed the combined-versioning-and-log branch from 6c3cef6 to 252fa85 Compare November 27, 2024 12:13
Copy link
Member

@carpawell carpawell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Conflicts.


// combinedPrefix is the prefix that Protobuf message can't start with,
// it reads as "field number 15 of type 7", but there is no type 7 in
// the system (and we usually don't have 15 fields). ZSTD magic is also
// different.
combinedPrefix = 0x7f

// combinedLenSize is sizeof(uint32), length is a serialized 32-bit BE integer.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// combinedLenSize is sizeof(uint32), length is a serialized 32-bit BE integer.
// combinedLenSize is sizeof(uint32), length of a serialized 32-bit BE integer.

no?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, to be fixed.

Our real storages have nothing to report.

Signed-off-by: Roman Khimov <[email protected]>
In case they want to log something.

Signed-off-by: Roman Khimov <[email protected]>
It is important to know which type of the writer is used, they differ in
characteristics.

Signed-off-by: Roman Khimov <[email protected]>
Yeah, lengthy names, but it's better for future extensions.

Signed-off-by: Roman Khimov <[email protected]>
We may want to have something more complex in future versions (like #2925 or
others) and we don't have a lot of safe bytes to use in place of 0x7F. So
let's have a byte for format version.

This is an incompatible change, but this code was never in production.

Signed-off-by: Roman Khimov <[email protected]>
When we already know that the file is combined (seen the prefix at least
once, isCombined is true) we expect all subsequent entries to be proper
combined ones as well. If they're not (wrong prefix) --- something is wrong
with the file and returning it as is won't help.

Signed-off-by: Roman Khimov <[email protected]>
@roman-khimov roman-khimov force-pushed the combined-versioning-and-log branch from 252fa85 to b79dcfa Compare November 28, 2024 05:51
@roman-khimov roman-khimov merged commit 2c087f3 into master Nov 28, 2024
22 checks passed
@roman-khimov roman-khimov deleted the combined-versioning-and-log branch November 28, 2024 06:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants